kola: promote concept of sdk.LocalBuild throughout #1283
Conversation
jlebon
force-pushed
the
pr/cosa-kola
branch
3 times, most recently
from
a4e4fb5 to
68c7087
Compare
|
Sorry, force-pushed a few times to tweak the commit message since there's one other key change I hadn't mentioned, which is:
|
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Yep this is the direction I wanted to go too, but I didn't do it at the time because it would have been a conflict-fest with the testiso work.
src/cmd-kola
Outdated
| kolaargs.extend(['-p', 'qemu-unpriv']) | ||
|
|
||
| if args.build is not None: | ||
| kolaargs.extend(['--cosa-build', args.build]) |
There was a problem hiding this comment.
Why not just call the argument in kola --build? Then we could drop this right?
There was a problem hiding this comment.
I did this (and actually went one step further renaming --cosa-workdir to just --workdir) because yeah, kola now lives in cosa and we should reflect that.
Re. dropping the --build dir, I left that alone for now because the passthrough unknown args hack falls apart for switches which take values. It'll all be resolved anyway once cosa kola "$@" is just an exec kola "$@".
|
Although...this is tricky because for IaaS cases we can support directly using a |
arithx
left a comment
arithx
left a comment
There was a problem hiding this comment.
Idea makes sense to me, not sure if we have any CI machinery / pipelines that need to coordinate changes with this.
Hmm, or maybe in that case we want to make |
|
With |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Overall this makes sense I think, though maybe what we really want is something where we just have a
type Build struct {
base string
meta cosa.Meta
}
And base could be a URL or a local file path - in the URL case we'd dynamically download the required artifacts.
Instead of keeping separate the build metadata object and the build
directory, let's instead raise the visibility of `sdk.LocalBuild`, which
bundles those two things together already.
What I initially set out to solve was that `cosa kola --build ID` no
longer worked, because we weren't passing `--cosa-build` to kola
anymore. But even if we did, it's silly to have `cosa kola` dig into
build dirs to fetch the path to the `meta.json` when `kola` now knows
how to do that.
Concretely, this changes three things:
1. we add a `--cosa-workdir` to allow specifying a cosa dir other than
the current working dir.
2. the `--cosa-build` argument is now a build ID, not a path to a
`meta.json`.
3. `kola.CosaBuild` is now a `LocalBuild` so that we also have access to
the build directory everywhere.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arithx, cgwalters, jlebon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
Instead of keeping separate the build metadata object and the build
directory, let's instead raise the visibility of
sdk.LocalBuild, whichbundles those two things together already.
What I initially set out to solve was that
cosa kola --build IDnolonger worked, because we weren't passing
--cosa-buildto kolaanymore. But even if we did, it's silly to have
cosa koladig intobuild dirs to fetch the path to the
meta.jsonwhenkolanow knowshow to do that.
Concretely, this changes three things:
--cosa-workdirto allow specifying a cosa dir other thanthe current working dir.
--cosa-buildargument is now a build ID, not a path to ameta.json.kola.CosaBuildis now aLocalBuildso that we also have access tothe build directory everywhere.